Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚫 Create failing test cases for downstream async middleware/listeners #337

Closed
wants to merge 1 commit into from

Conversation

tteltrab
Copy link
Contributor

@tteltrab tteltrab commented Dec 11, 2019

Summary

Demonstrate failing build/failing test cases which we can eventually make pass hopefully? Wanted to show the failing tests with a build run. This is partially here to demonstrate #338

Requirements (place an x in each [ ])

@aoberoi
Copy link
Contributor

aoberoi commented Dec 13, 2019

@tteltrab this is really useful at demonstrating the issue! thanks for taking the time to write these tests.

i like the idea of keeping these kinds of tests in the project's test suite. i think when we land it, we should call them integration tests (and move them to a different file/directory where that's clear).

now, for how we might fix this...

one option we have (which is discussed in the thread you linked to) is to use an arbitrary await delay(n) at some point after the await handler(request) but before you begin to make assertions. it's been noted that this approach feels fragile because the choice of n is arbitrary, and there's no real signal that the middleware/listener chain(s) execution completed.

another option is to redesign the way processMiddleware works so that we have that signal, and can make assertions afterwards. there are several downsides to this option:

  1. processMiddleware dispatches to all listener middleware chains at the same time. if each of them returned a promise, we'd have to wait for all the promises to resolve/reject (like Promise.finally()) before eventually resolving/rejecting to bubble up the global middleware chain. this could be a considerable performance hit.
  2. it becomes trickier to write well-behaved middleware/listeners. today, the return value of a listener doesn't matter. but if we made this change then every developer would have to manager the returned Promise, and make sure it resolved/rejected in the appropriate amount of time. specifically, there's a temptation to just await any function calls that return a Promise (because its the easiest thing to do and avoids callback and/or then-chaining hell). this actually makes the bubble up the middleware chain slower than necessary. the best practice would be to await everything until you're willing to call ack(), and then async work inside an async function should not use the await keyword. that feels wrong and error prone.
  3. it would be a breaking change. since the return value of listeners/middleware exhibit new behavior, one could see this as not backwards compatible.

one more option is in your downstream testing code, trust that the framework will call your listeners in the way it specifies (since the framework itself is well-tested [or will be 😉]). you can write your test listeners/middleware to return a promise, and then run your assertions after that promise is resolved. i'm thinking something like this:

  it('correctly waits for async listeners', async () => {
    let changed = false;
    let response;
    await new Promise((resolve) => {
      app.message(message, async () => {
        await delay(100);
        changed = true;
        resolve();
      });
      response = await handler(request);
    });
    
    assert.equal(response.statusCode, 200);
    assert.isTrue(changed);
  });

these are the only options i've been able to think of so far. open to discussing these or hearing more!

@barlock
Copy link
Contributor

barlock commented Jan 24, 2020

I added these tests to App.spec.ts here 269f462#diff-a7a8d4758cfeb70999a809869b45fca0R353

Probably can close this now.

@tteltrab
Copy link
Contributor Author

This was incorporated and will be iterated on in #337

@tteltrab tteltrab closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants